inspector: Allow require in Runtime.evaluate #8837

jkrems · 2016-09-28T20:55:31Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

inspector

Description of change

Being able to require modules from the console can be very handy while debugging issues. This allows to do it even when require is no longer in scope or the process isn't currently paused. It also creates the foundation for adding other command line API methods in the future (if more use cases come up).

jkrems · 2016-09-28T21:00:01Z

I wasn't sure if this warrants a mention in the docs. So far I assume that require is something people will implicitly assume works (the behavior is roughly the same as in the repl / node -e / etc.).

Fishrock123 · 2016-09-28T22:28:13Z

lib/internal/bootstrap_node.js

@@ -225,12 +225,27 @@
    global.setTimeout = timers.setTimeout;
  }

+  function setupConsoleAPI(addCommandLineAPIMethod) {
+    addCommandLineAPIMethod('require', function require(request) {


Instead of passing, this could just access process.inspector.addCommandLineAPIMethod directly, whcih would be preferred.

nit: I would move this below setupGlobalConsole

Moved setupConsoleAPI below and renamed it to setupInspectorAPI (closer to inspector.addCommandLineAPIMethod). Also switched to accessing the method from process instead of passing it.

jasnell · 2016-09-30T15:02:32Z

lib/internal/bootstrap_node.js

@@ -249,6 +250,21 @@
    });
  }

+  function setupInspectorAPI() {
+    var addCommandLineAPIMethod = process.inspector.addCommandLineAPIMethod;


nit: const?

jasnell · 2016-09-30T15:03:56Z

test/inspector/test-inspector.js

@@ -151,6 +151,37 @@ function testInspectScope(session) {
  ]);
 }

+function testCommandLineAPI(session) {
+  console.log('[test]', 'Verify that command line API was injected');


nit: I'd prefer if the tests did not include unnecessary console.log statements

Same here - should I remove the console.log from the other functions in this file as well? Only added it for consistency with the existing code.

Removed the console.log I added already.

jasnell

LGTM if CI is happy and @Fishrock123's concerns are met.

alexkozy · 2016-09-30T15:12:21Z

src/inspector_agent.cc

+
+void AgentImpl::AddAPIMethod(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  auto env = Environment::GetCurrent(info);
+  v8::Local<v8::Object> console_api = env->inspector_console_api_object();


Could you check arguments here and ThrowError if something wrong in the same way like in InspectorWrapConsoleCall?

alexkozy · 2016-09-30T15:13:59Z

src/inspector_agent.cc

@@ -573,6 +585,16 @@ void AgentImpl::InstallInspectorOnProcess() {
  v8::Local<v8::Object> inspector = v8::Object::New(env->isolate());
  READONLY_PROPERTY(process, "inspector", inspector);
  env->SetMethod(inspector, "wrapConsoleCall", InspectorWrapConsoleCall);
+  env->SetMethod(inspector, "addCommandLineAPIMethod", AddAPIMethod);


nit: I'd prefer InspectorAddCommandLineAPIMethod or AddCommandLineAPIMethod as a name of this function.

Yeah, the method name might have gotten shorter after I discovered the 80 char limit... Went back to a more descriptive name & more line breaks.

alexkozy · 2016-09-30T15:15:03Z

src/env.h

@@ -240,6 +240,7 @@ namespace node {
  V(domains_stack_array, v8::Array)                                           \
  V(fs_stats_constructor_function, v8::Function)                              \
  V(generic_internal_field_template, v8::ObjectTemplate)                      \
+  V(inspector_console_api_object, v8::Object)                                 \


Just to check, is this object accessible from JS code?

this isn't an object per-se, it is not exposed to JS

As @Fishrock123 said, this is just used as a bucket of named persistent values from native code, it's never directly exposed to JS.

alexkozy · 2016-09-30T15:15:56Z

lib/internal/bootstrap_node.js

@@ -231,6 +231,7 @@
    if (process.inspector) {
      inspectorConsole = global.console;
      wrapConsoleCall = process.inspector.wrapConsoleCall;
+      setupInspectorAPI();


nit: I'd prefer API -> commandLineAPI. API sounds too broad for me.

nah, I think this is fine.

I'm with @ak239, setupInspectorCommandLineAPI() is a more descriptive name.

alexkozy · 2016-09-30T15:17:34Z

lib/internal/bootstrap_node.js

@@ -249,6 +250,21 @@
    });
  }

+  function setupInspectorAPI() {
+    var addCommandLineAPIMethod = process.inspector.addCommandLineAPIMethod;


Can we remove this method from inspector object right here to be sure that it won't leak?

Yep, will do.

alexkozy · 2016-09-30T15:21:19Z

src/inspector_agent.cc

@@ -253,6 +253,7 @@ class AgentImpl {
  static void WriteCbIO(uv_async_t* async);

  void InstallInspectorOnProcess();
+  static void AddAPIMethod(const v8::FunctionCallbackInfo<v8::Value>& info);


Could you define this method outside of AgentImpl?

Moved this out and renamed (see below).

Fishrock123 · 2016-09-30T20:04:55Z

lib/internal/bootstrap_node.js

@@ -249,6 +250,21 @@
    });
  }

+  function setupInspectorAPI() {
+    const addCommandLineAPIMethod = process.inspector.addCommandLineAPIMethod;
+    addCommandLineAPIMethod('require', function require(request) {


is this callback called synchronously? if not, I need to inspect the startup timing a bit more

The line you're commenting on is executed synchronously during bootup. From my understanding that should prevent any debug client from connecting (and calling Runtime.evaluate) before we registered the API method.

Sounds good to me.

jkrems · 2016-10-03T02:41:14Z

Sorry for force-pushing the last couple of times, missed that sentence in the contribution guide.

Pushed a commit to address the last round of feedback.

gibfahn · 2016-10-03T11:37:46Z

@jkrems I don't think force-pushing is a problem, AFAIK it's a matter of personal preference. I like to use --amend so the commits look tidier, but either way should be fine. Which sentence in the contribution guide were you referring to?

alexkozy · 2016-10-03T14:47:52Z

thanks, lgtm

addaleax · 2016-10-03T14:53:05Z

src/inspector_agent.cc

@@ -567,12 +578,30 @@ void AgentImpl::WaitForDisconnect() {
                           v8::ReadOnly).FromJust();                          \
  } while (0)

+void AddCommandLineAPIMethod(const v8::FunctionCallbackInfo<v8::Value>& info) {


This can be static, right? If so, it should probably be.

My C++ is a bit rusty, so the following might be just me missing one of the "things static can mean": It's no longer a class member but a top-level function as of the latest commit.

@jkrems Yeah, for top-level functions (i.e. in namespace scope, like this one), adding static basically means that the function will only be visible in the file in which it is defined. I think it’s generally good practice to have any top-level functions either appear in a header file to make clear that they are shared between files/part of the public C++ API, or be static to indicate that they are intended to be local. You can grep src/ for FunctionCallbackInfo<Value>& args, most of these functions are static.

(Also, yeah, no doubt that assigning multiple distinct meanings to static is not an ideal thing for C++. 😄)

That's why I have a mild preference for inline. It does the same thing as static but there is never any ambiguity as to whether it gets applied to a function or a variable declaration because it's only valid for the former. Helpful when grepping around.

@addaleax & @bnoordhuis: Thanks a lot for the explanation. I'm pretty sure I learned & forgot this before. Once or twice.

I updated this function and also made the other two exposed C++ methods (InspectorConsoleCall/InspectorWrapConsoleCall) static so the style is consistent inside of the file. Went with static over inline because it seemed to be the dominant style in other *.cc files.

addaleax · 2016-10-03T14:54:17Z

src/inspector_agent.cc

+  auto env = Environment::GetCurrent(info);
+
+  if (info.Length() != 2 || !info[0]->IsString() || !info[1]->IsFunction()) {
+    return env->ThrowError("inspector.addCommandLineAPIMethod takes exactly "


env->ThrowTypeError?

I thought so but it was using ThrowError for the same thing above. Should I change both or just my copy?

@jkrems It’s probably okay change both. :)

jkrems · 2016-10-03T15:36:41Z

@gibfahn Here's the quote I was referring to:

If there are comments to address, apply your changes in a separate commit and push that to your branch.

jkrems · 2016-10-17T17:38:49Z

Anything left to address here?

jkrems · 2016-12-13T02:36:26Z

Rebased on origin/master & make -j8 test (the only conflict was another test case that was added to test/inspector/test-inspector.js in the meantime).

bnoordhuis · 2016-12-13T11:20:55Z

lib/internal/bootstrap_node.js

@@ -231,6 +231,7 @@
    if (process.inspector) {
      inspectorConsole = global.console;
      wrapConsoleCall = process.inspector.wrapConsoleCall;
+      setupInspectorAPI();


I'm with @ak239, setupInspectorCommandLineAPI() is a more descriptive name.

bnoordhuis · 2016-12-13T11:46:44Z

lib/internal/bootstrap_node.js

+      module.filename = path.join(cwd, name);
+      module.paths = Module._nodeModulePaths(cwd);
+      return module.require(request);
+    });


Shouldn't module be defined outside the function? I think this breaks require('foo') === require('foo'), i.e., that the second require returns the cached module instance.

The module cache is maintained by Module, so individual module instances shouldn't matter. It does influence module.parent (e.g. two modules that were loaded initially using the global require will not share the same module.parent). But that seemed less important.

Adding a test case to verify.

Somebody is bound to run into that sooner or later. If it's easy to avoid, I'd fix it now.

Somebody is bound to run into that sooner or later.

Can you clarify what "it" is in this context? There's now a test case that verifies that it doesn't break require('foo') === require('foo'), unless I misunderstood your initial concern?

That was my initial concern, yes, but you mentioned that the .parent property will be different for each require() call. If that's easy to avoid, I'd do it now.

Gotcha, will add additional tests for require('foo').parent === require('bar').parent and fix this.

bnoordhuis · 2016-12-13T11:49:01Z

src/inspector_agent.cc

+    for (uint32_t i = 0; i < properties->Length(); ++i) {
+      v8::Local<v8::Value> key = properties->Get(i);
+      target->Set(key, console_api->Get(key));
+    }


Can you use the GetOwnPropertyNames, Get and Set overloads that take a context as their first argument and return a Maybe/MaybeLocal?

bnoordhuis · 2016-12-13T11:49:45Z

src/inspector_agent.cc

+  }
+
+  v8::Local<v8::Object> console_api = env->inspector_console_api_object();
+  console_api->Set(info[0], info[1]);


bnoordhuis · 2016-12-13T11:54:50Z

test/inspector/test-inspector.js

+      }, (message) => assert.strictEqual('object', message['result']['value'])
+    ],
+  ]);
+}


Can you check that:

require(name) twice returns the same object, and

that delete require.cache[require.resolve(name)] followed by require again returns a new object, and

that calling require again returns the same object?

name could be e.g. const name = path.join(common.fixturesDir, 'empty');

I'll try. I'm not sure if require.cache is exposed globally by this PR. Since the require goes through the normal module.require, is there any reason to believe that deleting the require cache won't affect it?

gibfahn · 2016-12-13T17:19:30Z

@jkrems FYI, this is no longer in CONTIBUTING.md, so feel free to squash or not, whatever you prefer...

jkrems · 2016-12-13T19:41:22Z

Updated, went with adding it as another commit because consistency.

TimothyGu · 2017-07-25T05:20:37Z

I'm willing to take this up and fix whatever is needed to get this in, so reopening.

TimothyGu · 2017-07-25T14:04:31Z

@bnoordhuis Comments addressed, so PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/9344/

bnoordhuis · 2017-07-25T14:48:30Z

lib/internal/bootstrap_node.js

-    consoleAPIModule.paths = Module._nodeModulePaths(cwd);
+    const consoleAPIModule = new Module('<inspector console>');
+    consoleAPIModule.paths =
+      Module._nodeModulePaths(cwd).concat(Module.globalPaths);


Teeny tiny nit: four space indent.

@bnoordhuis Does the four-space indent rule apply even to JS?

It's used pretty consistently in both JS and C++ code. In fact, I thought the JS linter enforced it, even.

lib/buffer.js consistently uses 2-space indentation (grep for =$), but either way it's fine for me.

TimothyGu · 2017-07-31T01:11:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9412/

TimothyGu · 2017-08-06T07:18:27Z

Rebased. I intend to land this soon if CI doesn't have any problems with that.

CI: https://ci.nodejs.org/job/node-test-commit/11573/

TimothyGu · 2017-08-07T02:06:57Z

Landed in 5fd2f03.

Some parts were written by Timothy Gu <timothygu99@gmail.com>. PR-URL: #8837 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Notable changes * **Inspector** * `require()` is available in the inspector console now. [#8837](#8837) * **N-API** * New APIs for creating number values have been introduced. [#14573](#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](#14558) PR-URL: #14811

Notable changes * **HTTP2** * Experimental support for the built-in `http2` has been added via the `--expose-http2` flag. [#14239](#14239) * **Inspector** * `require()` is available in the inspector console now. [#8837](#8837) * Multiple contexts, as created by the `vm` module, are supported now. [#14465](#14465) * **N-API** * New APIs for creating number values have been introduced. [#14573](#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](#14558) PR-URL: #14811

Notable changes * **HTTP2** * Experimental support for the built-in `http2` has been added via the `--expose-http2` flag. [#14239](nodejs/node#14239) * **Inspector** * `require()` is available in the inspector console now. [#8837](nodejs/node#8837) * Multiple contexts, as created by the `vm` module, are supported now. [#14465](nodejs/node#14465) * **N-API** * New APIs for creating number values have been introduced. [#14573](nodejs/node#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](nodejs/node#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](nodejs/node#14558) PR-URL: nodejs/node#14811

gibfahn · 2018-01-15T22:11:38Z

Release team were -1 on landing on v6.x, if you disagree let us know.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. inspector Issues and PRs related to the V8 inspector protocol labels Sep 28, 2016

Fishrock123 suggested changes Sep 28, 2016

View reviewed changes

jkrems force-pushed the jk-global-require branch from 950a6e0 to 4d578e8 Compare September 28, 2016 23:08

jasnell reviewed Sep 30, 2016

View reviewed changes

jasnell approved these changes Sep 30, 2016

View reviewed changes

jkrems force-pushed the jk-global-require branch from 4d578e8 to aa5f323 Compare September 30, 2016 15:21

alexkozy suggested changes Sep 30, 2016

View reviewed changes

Fishrock123 reviewed Sep 30, 2016

View reviewed changes

Fishrock123 approved these changes Sep 30, 2016

View reviewed changes

alexkozy approved these changes Oct 3, 2016

View reviewed changes

addaleax reviewed Oct 3, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

bnoordhuis mentioned this pull request Nov 23, 2016

--inspect inspector require file nodejs/help#375

Closed

jkrems force-pushed the jk-global-require branch from 6cce8d3 to 92a3510 Compare December 13, 2016 02:33

nodejs-github-bot added the dont-land-on-v7.x label Dec 13, 2016

bnoordhuis requested changes Dec 13, 2016

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Dec 13, 2016

TimothyGu reopened this Jul 25, 2017

TimothyGu force-pushed the jk-global-require branch from 6a6619c to 268790f Compare July 25, 2017 14:02

bnoordhuis approved these changes Jul 25, 2017

View reviewed changes

TimothyGu self-assigned this Jul 31, 2017

TimothyGu removed the stalled Issues and PRs that are stalled. label Jul 31, 2017

TimothyGu force-pushed the jk-global-require branch from 268790f to a77c0fa Compare July 31, 2017 01:04

Jan Krems and others added 3 commits August 6, 2017 15:13

inspector: Allow require in Runtime.evaluate

812e351

Address comments

7a4d57b

Fix indentation

0a183cd

TimothyGu force-pushed the jk-global-require branch from a77c0fa to 0a183cd Compare August 6, 2017 07:16

TimothyGu closed this Aug 7, 2017

jkrems deleted the jk-global-require branch August 7, 2017 14:11

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 13, 2017

addaleax mentioned this pull request Aug 13, 2017

v8.4.0 proposal #14811

Merged

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

inspector: Allow require in Runtime.evaluate #8837

inspector: Allow require in Runtime.evaluate #8837

Conversation

jkrems commented Sep 28, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jkrems commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkozy Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems commented Oct 3, 2016

gibfahn commented Oct 3, 2016

alexkozy commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkrems commented Oct 3, 2016

jkrems commented Oct 17, 2016

jkrems commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Dec 13, 2016

jkrems commented Dec 13, 2016

TimothyGu commented Jul 25, 2017

TimothyGu commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Jul 31, 2017

TimothyGu commented Aug 6, 2017

TimothyGu commented Aug 7, 2017

gibfahn commented Jan 15, 2018

jkrems commented Sep 28, 2016 •

edited

Loading

jkrems Sep 28, 2016 •

edited

Loading

jasnell Sep 30, 2016 •

edited

Loading

jkrems Oct 3, 2016 •

edited

Loading

alexkozy Sep 30, 2016 •

edited

Loading

jkrems commented Dec 13, 2016 •

edited

Loading